-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Xcode warnings in React-Core pod #29622
Conversation
Base commit: f8e5423 |
cc @alloy |
Base commit: f8e5423 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, thanks 👍
No approval or request for changes just yet.
.gitignore
Outdated
@@ -88,6 +88,7 @@ RNTester/build | |||
/template/ios/Pods/ | |||
/template/ios/Podfile.lock | |||
/RNTester/Gemfile.lock | |||
/RNTester/vendor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What’s this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vendor
directory came for running bundle install --path vendor
, which I did to avoid installing the gems as system gems. I can remove it from this PR. It would be nice to have some better instructions on how to set up the development environment for newbies.
React/Base/RCTBridge.m
Outdated
[self reloadWithReason:@"Command"]; | ||
#pragma clang diagnostic pop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this is being tracked in an issue? Seeing as it will now go silent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling this out. I went ahead and took a stab at moving off these deprecated methods. Let me know if I should revert that and just file an issue instead.
React/Base/RCTJSStackFrame.m
Outdated
@@ -100,7 +100,7 @@ + (instancetype)stackFrameWithLine:(NSString *)line | |||
file:file | |||
lineNumber:[lineNumber integerValue] | |||
column:[column integerValue] | |||
collapse:@NO]; | |||
collapse:@NO.integerValue]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a smell. Can the collapse
var be made a BOOL
instead, at least internally to this class? If not, then at least it seems better to just pass 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Shouldn't be a huge change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to BOOL
.
React/CxxBridge/RCTCxxBridge.mm
Outdated
@@ -1125,6 +1128,8 @@ - (void)reloadWithReason:(NSString *)reason | |||
RCTTriggerReloadCommandListeners(reason); | |||
} | |||
|
|||
#pragma clang diagnostic pop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, this should be tracked in some way. Perhaps the alternative to an issue is to leave comments pointing to these call-sites from the deprecated implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since RCTCxxBridge
inherits from RCTBridge
, I went ahead and just removed these overrides, since RCTBridge
now has the same implementation as this (minus the warn log).
React/Modules/RCTUIManagerUtils.m
Outdated
@@ -8,6 +8,7 @@ | |||
#import "RCTUIManagerUtils.h" | |||
|
|||
#import <libkern/OSAtomic.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this import go too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
@@ -98,6 +99,6 @@ void RCTUnsafeExecuteOnUIManagerQueueSync(dispatch_block_t block) | |||
NSNumber *RCTAllocateRootViewTag() | |||
{ | |||
// Numbering of these tags goes from 1, 11, 21, 31, ..., 100501, ... | |||
static int64_t rootViewTagCounter = -1; | |||
return @(OSAtomicIncrement64(&rootViewTagCounter) * 10 + 1); | |||
static _Atomic int64_t rootViewTagCounter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs:
Atomically replaces the value pointed by obj with the result of addition of arg to the old value of obj, and returns the value obj held previously.
So that’s why this should initialise with 0
instead of -1
, like previously was the case.
Do you know if there are tests that cover this API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any tests per se, but the RNTester app crashed immediately when this was initialized to -1
.
React/Profiler/RCTProfile.m
Outdated
[RCTProfilingBridge() reloadWithReason:@"Profiling controls"]; | ||
#pragma clang diagnostic pop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess some questions are actual request for changes 😄
8f6d0c8
to
694f056
Compare
Thanks for the review @alloy. I think I've resolved all of the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for your work!
/cc @TheSavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@appden has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by Jayme Deffenbaugh in cb719a1. When will my fix make it into a release? | Upcoming Releases |
@@ -36,6 +36,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
@property (nonatomic, assign) NSTimeInterval loadingViewFadeDelay; | |||
@property (nonatomic, assign) NSTimeInterval loadingViewFadeDuration; | |||
|
|||
- (instancetype)initWithSurface:(RCTSurface *)surface | |||
sizeMeasureMode:(RCTSurfaceSizeMeasureMode)sizeMeasureMode NS_UNAVAILABLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdeff Sorry, I had to remove this one before merging because it caused issues with Facebook's internal builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@appden Can you share details about the build failure?
Summary
With the upgrade to React Native 0.63, we started running into nullability warnings that were breaking our build. This PR fixes those nullability warnings as well as a few other warnings in React-Core.
Changelog
[iOS] [Fixed] - Fix xcodebuild warnings in React-Core
Test Plan
RCTAllocateRootViewTag
is the only real logic change in this PR. We throw an exception if the root view tag is not in the correct format, so this change seems safe after some basic manual testing in RNTester.